Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweaks to --strict-equality based on user feedback #6674

Merged
merged 8 commits into from Apr 27, 2019

Conversation

ilevkivskyi
Copy link
Member

Fixes #6607
Fixes #6608

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This should make --strict-equality generate many fewer false positives. Left some comments about minor things. You can merge this once you've addressed the feedback.

from typing import Union
x: Union[bytes, str]

b'abc' in x
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b'a' in 'b' fails at runtime. Should this generate an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is independent of this flag. The error message says "Non-overlapping container check ..." while in this example the check may return True. I think this can be tightened in typeshed, we can just define str.__contains__ as accepting str, because 42 in 'b' etc. all fail as well at runtime.

return False

def dangerous_comparison(self, left: Type, right: Type,
original_cont_type: Optional[Type] = None) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: original_cont_type was not immediately clear. Maybe rename to original_container or container_type?

# TODO: support other types (see has_member())?
return False

def has_bytes_component(self, typ: Type) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: This should be a module-level function.

@@ -1974,9 +1980,33 @@ def visit_comparison_expr(self, e: ComparisonExpr) -> Type:
assert result is not None
return result

def dangerous_comparison(self, left: Type, right: Type) -> bool:
def custom_equality_method(self, typ: Type) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: This should be a module-level function, since it doesn't depend on self.

if method and method.info:
return not method.info.fullname().startswith('builtins.')
return False
# TODO: support other types (see has_member())?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be good to support some additional types -- at least TupleType (use fallback). Maybe Any should return true as well?

"""Does this type have a custom __eq__() method?"""
if isinstance(typ, UnionType):
return any(self.custom_equality_method(t) for t in typ.items)
if isinstance(typ, Instance):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A really minor nit: maybe handle Instance first, since it's probably the most common case.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Thanks for the updates!

@ilevkivskyi
Copy link
Member Author

Opened python/typeshed#2937 for the str.__contains__() issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--strict-equality prohibits a valid check b'abc' in b'cde' --strict-equality should be more flexible
2 participants